Fix A11y EndExclusive Error for Move & Expand #7677

carlos-zamora · 2020-09-18T21:35:29Z

EndExclusive represents the end of the buffer. This is designed to not
point to any data on the buffer. UiaTextRange would point to this
EndExclusive and then attempt to move based on it. However, since it
does not point to any data, it could experience undefined behavior or
(inevitably) crash from running out of bounds.

This PR specifically checks for expansion and movement at that point,
and prevents us from moving beyond it. There are plans in the future to
define the "end" as the last character in the buffer. Until then, this
solution will suffice and provide correct behavior that doesn't crash.

Validation Steps Performed

Performed the referenced bugs' repro steps and added test coverage.

Closes MSFT-20458595
Closes #7663
Closes #7664

DHowett · 2020-09-18T21:54:52Z

did you figure out how we could end up with -1 in a uia endpoint?
it looks like this stops all movement at the end-degenerate. what about moving backwards?

DHowett · 2020-09-18T22:50:19Z

Build veyr mad

src/buffer/out/textBuffer.cpp

codeofdusk · 2020-09-19T22:15:54Z

CC @Simon818

DHowett · 2020-09-21T16:57:01Z

src/host/ut_host/TextBufferTests.cpp

@@ -2065,7 +2065,7 @@ void TextBufferTests::GetWordBoundaries()
        { { 79, 0 },    {{ 10, 0 },      { 5, 0 }} },

        // tests for second line of text
-        { {  0, 1 },     {{ 0, 1 },       { 0, 1 }} },
+        { {  0, 1 },     {{ 0, 1 },       { 5, 0 }} },


I'm not sure which of the code changes made this change. Is this good and righteous? It seems like it is because a11y defines the word to be the word + all the spaces after it, so you're inside the spaces at 0,1... so the word starts at 5,0... but why was this broken before?

This one was a mistake on my part back when I wrote these tests. You're right to be concerned but here's my reasoning on why this is the right fix:

You have the following text:

word other more words

You're current position is where the X is (0,1):

word other X more words

When we get the previous word, we're supposed to wrap around to encompass "other". Before, on accident, we would hit the left boundary and not wrap. So we would actually return the whitespace on the second line between the left boundary and "more".

how did this test not fail then? o_O

The test didn't fail because I wrote it incorrectly. It used to expect the whitespace. It should have expected the word on the previous line.

I can't figure out how to permalink code, but you want to look at textbuffer.cpp line 1000

if (target.X == GetSize().Left())

We were treating "Left" as "Origin" and refusing to expand left.

src/buffer/out/textBuffer.cpp

ghost · 2020-09-23T20:06:15Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

codeofdusk · 2020-09-23T20:10:04Z

Yeah. Gonna resolve this comment.

@carlos-zamora presumably in a follow-up PR?

carlos-zamora · 2020-09-23T20:13:14Z

Yeah. Gonna resolve this comment.

@carlos-zamora presumably in a follow-up PR?

Just a thought to play around with. This could be a part of #6453, since we'll hit that case often then.

DHowett · 2020-10-14T20:42:23Z

This fix was just released as part of Windows in Insider Build 20236!

`EndExclusive` represents the end of the buffer. This is designed to not point to any data on the buffer. UiaTextRange would point to this `EndExclusive` and then attempt to move based on it. However, since it does not point to any data, it could experience undefined behavior or (inevitably) crash from running out of bounds. This PR specifically checks for expansion and movement at that point, and prevents us from moving beyond it. There are plans in the future to define the "end" as the last character in the buffer. Until then, this solution will suffice and provide correct behavior that doesn't crash. ## Validation Steps Performed Performed the referenced bugs' repro steps and added test coverage. Closes MSFT-20458595 Closes #7663 Closes #7664 (cherry picked from commit 40893b2)

@carlos-zamora

In nvaccess/nvda#11428 (comment), Andre9642 reported a Conhost crash when switching to/from the alt buffer a few times with a Braille display connected. Upon further investigation, @carlos-zamora and I discovered that the FailFast was in `GetText`: more checks similar to #7677 were needed for this case. Tested with NVDA using a [Focus](https://www.freedomscientific.com/products/blindness/focus40brailledisplay/) Braille display. Improves nvaccess/nvda#11428

@carlos-zamora

In nvaccess/nvda#11428 (comment), Andre9642 reported a Conhost crash when switching to/from the alt buffer a few times with a Braille display connected. Upon further investigation, @carlos-zamora and I discovered that the FailFast was in `GetText`: more checks similar to #7677 were needed for this case. Tested with NVDA using a [Focus](https://www.freedomscientific.com/products/blindness/focus40brailledisplay/) Braille display. Improves nvaccess/nvda#11428 (cherry picked from commit 60437b8)

ghost · 2020-11-11T18:05:11Z

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-11-11T18:08:59Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

Fix A11y EndExclusive Error for Move & Expand

f5a831d

carlos-zamora requested a review from DHowett September 18, 2020 21:35

ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. labels Sep 18, 2020

DHowett requested changes Sep 18, 2020

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

audit mode + backwards tests

9d46d83

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 18, 2020

carlos-zamora added 3 commits September 18, 2020 16:47

EndInclusive should be right AND bottom inclusive

e9a444f

code format

8426076

moving backwards should wrap over boundary

af529e3

codeofdusk mentioned this pull request Sep 19, 2020

UIA in Console and WSL: crashes in some contexts nvaccess/nvda#11428

Closed

DHowett reviewed Sep 21, 2020

View reviewed changes

DHowett approved these changes Sep 22, 2020

View reviewed changes

DHowett requested a review from miniksa September 22, 2020 15:11

miniksa reviewed Sep 22, 2020

View reviewed changes

src/buffer/out/textBuffer.cpp Show resolved Hide resolved

miniksa approved these changes Sep 23, 2020

View reviewed changes

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 23, 2020

ghost merged commit 40893b2 into master Sep 23, 2020

ghost deleted the dev/cazamor/a11y/end-exclusive-out-of-bounds branch September 23, 2020 20:06

codeofdusk mentioned this pull request Sep 25, 2020

UIA: incorrect move to previous word #7742

Closed

DHowett mentioned this pull request Sep 28, 2020

Fix and test TextBuffer::MoveToPreviousWord() #7770

Merged

codeofdusk mentioned this pull request Oct 27, 2020

UIA: throw E_FAIL for out-of-bounds text #8052

Merged

This was referenced Nov 11, 2020

conhost crash (unknown cause) with UIA text range at {0, -1} #7664

Closed

conhost crash in narrator scan mode when jumping to heading 1 #7663

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix A11y EndExclusive Error for Move & Expand #7677

Fix A11y EndExclusive Error for Move & Expand #7677

carlos-zamora commented Sep 18, 2020 •

edited by DHowett

Loading

DHowett commented Sep 18, 2020

DHowett commented Sep 18, 2020

codeofdusk commented Sep 19, 2020

DHowett Sep 21, 2020

carlos-zamora Sep 21, 2020 •

edited

Loading

DHowett Sep 21, 2020

carlos-zamora Sep 21, 2020

carlos-zamora Sep 21, 2020

ghost commented Sep 23, 2020

codeofdusk commented Sep 23, 2020

carlos-zamora commented Sep 23, 2020

DHowett commented Oct 14, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

Fix A11y EndExclusive Error for Move & Expand #7677

Fix A11y EndExclusive Error for Move & Expand #7677

Conversation

carlos-zamora commented Sep 18, 2020 • edited by DHowett Loading

Validation Steps Performed

DHowett commented Sep 18, 2020

DHowett commented Sep 18, 2020

codeofdusk commented Sep 19, 2020

DHowett Sep 21, 2020

Choose a reason for hiding this comment

carlos-zamora Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

DHowett Sep 21, 2020

Choose a reason for hiding this comment

carlos-zamora Sep 21, 2020

Choose a reason for hiding this comment

carlos-zamora Sep 21, 2020

Choose a reason for hiding this comment

ghost commented Sep 23, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

codeofdusk commented Sep 23, 2020

carlos-zamora commented Sep 23, 2020

DHowett commented Oct 14, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

carlos-zamora commented Sep 18, 2020 •

edited by DHowett

Loading

carlos-zamora Sep 21, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.